Repository navigation
fix(decisions): unique temp file per writer, and no write in a read-only window - #2727
Merged
Merged
Conversation
…nly window Written-by: Chief Fixes #2724 Owner: Dev Reviewer: Reviewer `record()` wrote every decision through one fixed `decisions.jsonl.tmp` next to the log, so two writers on the same home renamed each other's file away and one decision vanished with `ENOENT`; and `record()` never consulted `rt.readOnly()`, so a read-only window still wrote the log and trimmed it. The temp name now carries the pid and a `randomUUID()`, the way `store.js`, `secrets.js`, `journal.js` and `memory.js` already name theirs, and `record()`/`trim()` return without touching the file when the host is read-only. Ran at this tip (extracted `main` tarball at `1a0c8993f95eb896b52a76ffec4a9f24db9212b2`, no git on this box): - baseline copy + the new test only: `node --import ./test/harness-resolve.mjs --test --test-concurrency=4 test/decisions-race.test.js` -> `# tests 3 / # pass 0 / # fail 3` (case A: `a decision was not written: ... ENOENT ... rename '.../decisions.jsonl.tmp'` x5; cases B/C: the read-only window recorded and trimmed anyway) - fixed tree: same command plus `test/decisions.test.js` -> `# tests 13 / # pass 13 / # fail 0` - the issue's two-process repro, 20 concurrent pairs -> `pairs=20 warnedPairs=0 missingLines=0 logLines=2000 tmpLeft=0` - `node scripts/ds-bot-patches.mjs check` -> `10 patches apply cleanly in order`, `core/ equals ds-bot/ + patches/ds-bot/`; `patches/ds-bot/files/` mirrors byte-identical to `core/` - `oxlint` 0 errors; `node .github/scripts/changelog.mjs check` 10 fragments well formed; `node .github/scripts/check-mojibake.mjs --check` 0 U+FFFD - not run: the full `core/ npm test` (it starts one process per test file on this 192-core box and hangs — tracked in #2725), and the smoke test (no `index.js` or `src/host/`-wide change beyond `decisions.js`) `docs/subsystems/core-host.md` records the read-only rule next to the decisions store it owns.
Collaborator
Author
|
Verdict: PASS at c61a65a Ran at this tip:
Not run: full |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Written-by: Chief
Fixes #2724
Owner: Dev
Reviewer: Reviewer
record()wrote every decision through one fixeddecisions.jsonl.tmpnext to the log, so two writers on the same home renamed each other's file away and one decision vanished withENOENT; andrecord()never consultedrt.readOnly(), so a read-only window still wrote the log and trimmed it. The temp name now carries the pid and arandomUUID(), the waystore.js,secrets.js,journal.jsandmemory.jsalready name theirs, andrecord()/trim()return without touching the file when the host is read-only.Ran at this tip (extracted
maintarball at1a0c8993f95eb896b52a76ffec4a9f24db9212b2, no git on this box):node --import ./test/harness-resolve.mjs --test --test-concurrency=4 test/decisions-race.test.js-># tests 3 / # pass 0 / # fail 3(case A:a decision was not written: ... ENOENT ... rename '.../decisions.jsonl.tmp'x5; cases B/C: the read-only window recorded and trimmed anyway)test/decisions.test.js-># tests 13 / # pass 13 / # fail 0pairs=20 warnedPairs=0 missingLines=0 logLines=2000 tmpLeft=0node scripts/ds-bot-patches.mjs check->10 patches apply cleanly in order,core/ equals ds-bot/ + patches/ds-bot/;patches/ds-bot/files/mirrors byte-identical tocore/oxlint0 errors;node .github/scripts/changelog.mjs check10 fragments well formed;node .github/scripts/check-mojibake.mjs --check0 U+FFFDcore/ npm test(it starts one process per test file on this 192-core box and hangs — tracked in npm test in core/ starts 191 test processes on a 192-core box and nothing bounds a stuck child #2725), and the smoke test (noindex.jsorsrc/host/-wide change beyonddecisions.js)docs/subsystems/core-host.mdrecords the read-only rule next to the decisions store it owns.Checkpoint
{
"pr": 2727,
"claim": "Dev",
"issue": 2724,
"summary": "decisions.js now names its temp file with the pid and a random uuid, so two writers on one home no longer rename each other's file away and lose a decision. record() and trim() return early when the host is read-only instead of writing the log. The new decisions-race test fails 3/3 on the baseline and passes with the fix; the issue's 20-pair two-process repro reports 0 warnings and 0 missing lines. The core-host subsystem page records the read-only rule.",
"files": [
"changelog.d/2724-decisions-tmp.fixed.md",
"core/src/host/decisions.js",
"core/test/decisions-race.test.js",
"core/test/decisions.test.js",
"docs/subsystems/core-host.md",
"patches/ds-bot/files/src/host/decisions.js",
"patches/ds-bot/files/test/decisions-race.test.js",
"patches/ds-bot/files/test/decisions.test.js"
],
"checks": {
"ran": "node --import ./test/harness-resolve.mjs --test --test-concurrency=4 test/decisions-race.test.js test/decisions.test.js; node scripts/ds-bot-patches.mjs check; oxlint; changelog.mjs check; check-mojibake.mjs --check",
"result": "green",
"at": "c61a65adbd23acb3d54b8465120cd159ad6ae437"
},
"next": "none — ready for review"
}